[eslint-miner] feat(eslint): add require-escaped-regexp-interpolation rule - #49106
Conversation
Adds a new custom ESLint rule that flags values interpolated into `new RegExp()` template-literal patterns that are not passed through a regex-escaping helper first. Unescaped interpolation of a value containing regex metacharacters can produce unintended matches or, with attacker-controlled input, a ReDoS-prone pattern. Evidence from actions/setup/js: - sanitize_content_core.cjs:494 interpolates `fc` and `fenceLen` directly into a RegExp pattern without escaping. - model_aliases.cjs:98 and interpolate_prompt.cjs:47 show the same shape (currently non-adversarial input, but the pattern is unenforced and easy to misuse with untrusted input in the future). Running `npm run lint:setup-js` surfaces exactly 2 new warnings (both at sanitize_content_core.cjs:494), confirming the rule is specific and low-noise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories. |
|
|
There was a problem hiding this comment.
Pull request overview
Adds a custom ESLint rule intended to detect unescaped interpolation in dynamically constructed regular expressions.
Changes:
- Implements and tests the new rule.
- Registers it with the ESLint plugin.
- Enables it as a warning for setup JavaScript linting.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts |
Implements interpolation detection. |
eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts |
Adds rule tests. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts:76
- The blanket loop also reports interpolations that intentionally inject validated regex source rather than literal text. In the enabled corpus,
glob_pattern_helpers.cjs:74interpolates a pattern produced byescapeRegexChars,model_aliases.cjs:35-40interpolates a constant regex fragment, and the citedfenceLenis a numeric quantifier bound; escaping these expressions is either unnecessary or changes the pattern. The rule therefore produces false positives well beyond unsafe literal interpolation and needs a way to distinguish/exempt intentional regex fragments and numeric syntax before being enabled.
for (const expr of patternArg.expressions) {
if (isRecognizedAsEscaped(expr)) continue;
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts:69
- Checking only the identifier text does not ensure this is the global
RegExp. A local parameter, import, variable, or class namedRegExpwill be linted as though it were the built-in constructor, producing false positives despite the rule's stated global-only scope. Check the ESLint scope for a local binding before reporting, as the existingrequire-new-url-try-catchrule does.
if (node.callee.type !== AST_NODE_TYPES.Identifier || node.callee.name !== "RegExp") return;
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| // Identifier/member names that indicate a value has already been passed through | ||
| // a regex-escaping helper before interpolation, e.g. `escapeRegExp(x)`, | ||
| // `escapeRegex(x)`, or a variable named `escapedFoo` / `ESCAPED_FOO`. | ||
| const ESCAPE_NAME_PATTERN = /escape/i; |
| "Interpolated value `{{expr}}` in `new RegExp()` template literal is not passed through a regex-escaping helper. " + | ||
| "Escape regex metacharacters before interpolating, e.g. `{{expr}}.replace(/[.*+?^${}()|[\\]\\\\]/g, \"\\\\$&\")`.", |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Great work! 🚀 This PR looks ready for review. Summary: The new
Quality signals:
This is ready for maintainer review!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: require-escaped-regexp-interpolation ESLint rule
Overall well-designed rule with low false-positive risk. One correctness bug found.
Blocking issue: ESCAPE_NAME_PATTERN = /escape/i uses a substring match that inadvertently matches unescape and unescapedValue identifiers, causing false negatives (unescaped values silently pass). Fix with a negative lookbehind: /(?<![a-z])escape/i.
A test case covering new RegExp(\${unescapedValue}`)` as invalid would confirm the fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.7 AIC · ⊞ 5.3K
| // Identifier/member names that indicate a value has already been passed through | ||
| // a regex-escaping helper before interpolation, e.g. `escapeRegExp(x)`, | ||
| // `escapeRegex(x)`, or a variable named `escapedFoo` / `ESCAPED_FOO`. | ||
| const ESCAPE_NAME_PATTERN = /escape/i; |
There was a problem hiding this comment.
Bug: ESCAPE_NAME_PATTERN matches unescape and unescaped* identifiers (false negatives)
/escape/i is a substring match, so it inadvertently matches names like unescape, unescapedValue, or any identifier containing escape as a substring that is not actually an escaping helper:
const ESCAPE_NAME_PATTERN = /escape/i;
ESCAPE_NAME_PATTERN.test('unescapedValue'); // true — false negative!
ESCAPE_NAME_PATTERN.test('unescape'); // true — false negative!This means new RegExp(\${unescapedValue}`)` would be silently skipped instead of reported.
Suggested fix — use a negative lookbehind to exclude the un prefix:
const ESCAPE_NAME_PATTERN = /(?<![a-z])escape/i;This matches escapeRegExp, escapeRegex, ESCAPED_FOO, utils.escapeRegex but not unescape or unescapedValue.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 96/100 — Excellent
📊 Metrics (8 tests)
Test BreakdownThe
Test inflation ratio of 1.1:1 is lean and appropriate for an ESLint rule. Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
- False-negative risk in escape detection:
ESCAPE_NAME_PATTERN = /escape/imatchesunescapedValue,UNESCAPED_FOO, and the built-inunescape()— silently skipping those interpolations as if they were safe. - Missing test coverage for the false-negative case: no test verifies that
unescapedValueis actually flagged. - Missing README entry: every other rule in
eslint-factoryhas a table row and section inREADME.md; this rule has none.
Positive Highlights
- ✅ Well-motivated rule backed by real corpus scanning with precise, low-noise validation (exactly 2 warnings).
- ✅ Comprehensive test suite: 8 cases covering all valid/invalid branches.
- ✅ Clean implementation — helper functions are small, well-named, and well-documented.
- ✅ Security type (
problem) and detailed error message with an inline fix example.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.6 AIC · ⊞ 7K
Comment /matt to run again
| // a regex-escaping helper before interpolation, e.g. `escapeRegExp(x)`, | ||
| // `escapeRegex(x)`, or a variable named `escapedFoo` / `ESCAPED_FOO`. | ||
| const ESCAPE_NAME_PATTERN = /escape/i; | ||
|
|
There was a problem hiding this comment.
[/tdd] ESCAPE_NAME_PATTERN = /escape/i will also match names like unescapedValue, UNESCAPED_FOO, and the built-in unescape() — all of which are not escape helpers and would silently suppress a warning (false negative).
💡 Suggested fix
Narrow the pattern to only match names that start with or contain a positive "escape" signal, e.g.:
// Only match names that begin with "escape" (case-insensitive) or contain "EscapeReg"
const ESCAPE_NAME_PATTERN = /^escape|escapeReg/i;Alternatively, enumerate accepted helper names (like escapeRegExp, escapeRegex) for zero false negatives.
Add a test case:
// Should NOT be treated as safe — contains "escape" but is a bad signal:
{ code: "new RegExp(`${unescapedValue}`);" }@copilot please address this.
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The escapedValue naming-convention test case (new RegExp(\^${escapedValue}$`)) passes as valid, but there is no corresponding test that unescapedValueis correctly *flagged* as invalid. Without this, a regression in the/escape/i` pattern (e.g. after narrowing it) would go undetected.
💡 Add a companion invalid test
it("invalid: name starting with unescaped should NOT be treated as safe", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "new RegExp(`^${unescapedValue}$`);",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});@copilot please address this.
| export const requireEscapedRegexpInterpolationRule = createRule({ | ||
| name: "require-escaped-regexp-interpolation", | ||
| meta: { | ||
| type: "problem", |
There was a problem hiding this comment.
[/grill-with-docs] The README.md for eslint-factory documents every other rule with a table entry and dedicated section (e.g. require-fetch-try-catch, no-duplicate-constant-values), but this new rule is not added. Users of the plugin have no way to discover the rule purpose, options, or how to suppress it.
💡 Add a README entry
Add a row to the rules table and a ### require-escaped-regexp-interpolation section following the pattern of existing rules, including: what it detects, motivation, and the escape-helper naming convention for suppression.
@copilot please address this.
| NewExpression(node) { | ||
| if (node.callee.type !== AST_NODE_TYPES.Identifier || node.callee.name !== "RegExp") return; | ||
|
|
||
| const patternArg = node.arguments[0]; |
There was a problem hiding this comment.
[/tdd] The rule only fires on new RegExp(...) where the callee is a bare Identifier named RegExp. A common pattern like new globalThis.RegExp(...) or const RE = RegExp; new RE(...) would be silently skipped. This is a documented design choice (low false-positive), but the scope limitation should be called out in the rule description or a comment — not silently.
💡 Consider a doc comment at the NewExpression handler
// NOTE: Only bare `new RegExp(...)` is matched; aliased or member-accessed
// constructors (globalThis.RegExp, re2.RegExp, etc.) are intentionally out of
// scope to keep false-positive rate low.
NewExpression(node) {This makes the deliberate scope explicit for future maintainers.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…n require-escaped-regexp-interpolation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review issues in commit
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom ESLint rule,
require-escaped-regexp-interpolation, toeslint-factorythat flags values interpolated intonew RegExp()template-literal patterns which are not passed through a regex-escaping helper first.Motivation / evidence
Scanning
actions/setup/js/**/*.cjsfound a recurring, unenforced pattern: building aRegExpfrom a template literal that interpolates a raw variable without escaping regex metacharacters first. This risks unintended matches (if the value contains.,*,+,(, etc.) and, with attacker-influenced input, ReDoS.Real occurrences in the corpus (not hypothetical):
actions/setup/js/sanitize_content_core.cjs:494—new RegExp(\^[${fc}]{${fenceLen},}\s*$`)interpolatesfcandfenceLen` unescaped.actions/setup/js/model_aliases.cjs:98—new RegExp(\(^|[-\s])${qualifier}($|[-\s])`)interpolatesqualifier` unescaped.actions/setup/js/interpolate_prompt.cjs:47—new RegExp(\\$\{${varName}\}`, "g")interpolatesvarName` unescaped.Running
npm run lint:setup-jswith the new rule enabled surfaces exactly 2 warnings, both atsanitize_content_core.cjs:494— confirming the rule is specific and low-noise (not overly aggressive).Rule design (low false-positive risk)
new RegExp(...)where the callee is literally the globalRegExpidentifier.TemplateLiteralwith at least one interpolated expression (plain string literals, non-interpolated templates, and identifier-only arguments likenew RegExp(somePattern)are never flagged — no dataflow analysis needed, keeping false positives near zero)./escape/ion the called function name (e.g.escapeRegExp(x),utils.escapeRegex(x)) or on the referenced identifier/property name (e.g.ESCAPED_NAME,escapedValue).Changes
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts— new rule implementation.eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts— 8 test cases (valid: no interpolation, escaped via helper call, escaped via naming convention, unrelatednewconstructors; invalid: unescaped loop var, unescaped function param, multiple unescaped, mixed escaped/unescaped).eslint-factory/src/index.ts— registers the new rule in the plugin.eslint-factory/eslint.config.cjs— enables the rule as"warn"foractions/setup/js/**/*.cjs.Validation
cd eslint-factory && npm install✅cd eslint-factory && npm run build✅ (tscclean)npx vitest run src/rules/require-escaped-regexp-interpolation.test.ts— 8/8 passednpm test(full suite) — 401/406 passed; the 5 failures are inrequire-fs-io-try-catch.test.tsand are pre-existing onmain(verified viagit stashbefore this change), unrelated to this PR.npm run lint:setup-js— runs clean structurally; new rule contributes exactly 2 real warnings, no new errors.Scope
No changes outside
actions/setup/js(read-only, scanned only) andeslint-factory(rule implementation/config). No Go code touched.